Skip to content

fix: MFA SMS one-time password accepted twice under concurrent login (GHSA-jpq4-7fmq-q5fj)#10448

Merged
mtrezza merged 3 commits intoparse-community:alphafrom
mtrezza:fix/GHSA-jpq4-7fmq-q5fj-v9
Apr 26, 2026
Merged

fix: MFA SMS one-time password accepted twice under concurrent login (GHSA-jpq4-7fmq-q5fj)#10448
mtrezza merged 3 commits intoparse-community:alphafrom
mtrezza:fix/GHSA-jpq4-7fmq-q5fj-v9

Conversation

@mtrezza
Copy link
Copy Markdown
Member

@mtrezza mtrezza commented Apr 24, 2026

Issue

MFA SMS one-time password accepted twice under concurrent login (GHSA-jpq4-7fmq-q5fj)

Tasks

  • Add tests
  • Add changes
  • Add security check
  • Add benchmark

@parse-github-assistant
Copy link
Copy Markdown

parse-github-assistant Bot commented Apr 24, 2026

🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review.

Tip

  • Keep pull requests small. Large PRs will be rejected. Break complex features into smaller, incremental PRs.
  • Use Test Driven Development. Write failing tests before implementing functionality. Ensure tests pass.
  • Group code into logical blocks. Add a short comment before each block to explain its purpose.
  • We offer conceptual guidance. Coding is up to you. PRs must be merge-ready for human review.
  • Our review focuses on concept, not quality. PRs with code issues will be rejected. Use an AI agent.
  • Human review time is precious. Avoid review ping-pong. Inspect and test your AI-generated code.

Note

Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect.

Caution

Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 24, 2026

📝 Walkthrough

Walkthrough

Centralizes optimistic-lock checks for authData into a new helper and broadens lockable values to include primitives and arrays; applies these checks in RestWrite and UsersRouter to prevent concurrent reuse of single-use MFA tokens. Adds a regression test asserting SMS OTPs are single-use under concurrent logins.

Changes

Cohort / File(s) Summary
Vulnerability Regression Test
spec/vulnerabilities.spec.js
Adds a concurrency regression test for SMS MFA: captures server-generated OTP via custom sendSMS, enrolls MFA, issues a login OTP, sends the same OTP in 10 parallel /login requests, asserts only one succeeds, and verifies server-side token consumption.
Optimistic Locking Core Logic
src/AuthDataLock.js, src/RestWrite.js
Introduces applyAuthDataOptimisticLock (and isLockableAuthDataValue), centralizes snapshotting of originalAuthData, and applies optimistic-lock WHERE constraints for changed authData fields (non-null primitives and arrays) across response and non-response update paths.
Users Router Locking
src/Routers/UsersRouter.js
Replaces inlined authData optimistic-lock building with a call to the shared helper to extend WHERE constraints for lockable authData fields, avoiding adapter-variant object/Date-like values.

Sequence Diagram(s)

sequenceDiagram
    participant Client1 as Client 1
    participant Client2 as Client 2
    participant ClientN as Client N
    participant Server as Auth Server
    participant DB as Database

    rect rgba(100,200,150,0.5)
    note over Client1,ClientN: Multiple clients submit same OTP concurrently
    Client1->>Server: POST /login (OTP=xyz)
    Client2->>Server: POST /login (OTP=xyz)
    ClientN->>Server: POST /login (OTP=xyz)
    end

    rect rgba(150,150,200,0.5)
    note over Server,DB: Server applies optimistic-lock constraints based on original authData
    Server->>DB: SELECT ... WHERE authData.mfa.token = "xyz" AND authData.* original-equality constraints
    DB-->>Server: Row found (one request) / Not found (others)
    end

    rect rgba(200,100,100,0.5)
    note over Server,DB: Atomic consume of token via UPDATE
    Server->>DB: UPDATE user SET authData.mfa.token = NULL (affected rows == 1)
    DB-->>Server: Update success / No rows updated
    Server-->>Client1: Login success
    Server-->>Client2: OTP rejected
    Server-->>ClientN: OTP rejected
    end
Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

Possibly Related PRs

🚥 Pre-merge checks | ✅ 5 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Description check ⚠️ Warning The pull request description is entirely missing; no content was provided by the author despite a detailed template being available for the repository. Add a comprehensive PR description following the provided template, including: a link/reference to the GHSA advisory, a clear explanation of the changes (optimistic locking for authData), confirmation that tests were added, and any security check completion status.
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Security Check ✅ Passed PR implements robust security fix for GHSA-jpq4-7fmq-q5fj by applying optimistic locking to prevent SMS MFA OTP token reuse under concurrent requests.
Engage In Review Feedback ✅ Passed PR demonstrates clear engagement with review feedback. mtrezza clarified concerns about sendSMS distinction and removed a no-op branch in RestWrite.js per reviewer feedback, with consensus reached before threads were resolved.
Title check ✅ Passed The PR title begins with 'fix:' prefix as required and clearly describes the security fix addressing a vulnerability where MFA SMS OTPs could be accepted multiple times under concurrent login attempts.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 24, 2026

Codecov Report

❌ Patch coverage is 96.29630% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.51%. Comparing base (f26700e) to head (0a9603f).
⚠️ Report is 3 commits behind head on alpha.

Files with missing lines Patch % Lines
src/AuthDataLock.js 94.73% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            alpha   #10448   +/-   ##
=======================================
  Coverage   92.51%   92.51%           
=======================================
  Files         192      193    +1     
  Lines       16820    16831   +11     
  Branches      234      234           
=======================================
+ Hits        15561    15572   +11     
  Misses       1236     1236           
  Partials       23       23           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/RestWrite.js (1)

746-778: Consider exporting these helpers to eliminate the duplicate implementation in UsersRouter.js.

src/Routers/UsersRouter.js (lines 317-343) re-implements the same lock-eligibility loop (isLockable + provider/field iteration + JSON.stringify comparison) inline. If this logic ever changes (e.g., new lockable type added, or the skip-on-Date rule refined), both sites must be kept in sync. Exporting applyAuthDataOptimisticLock (or a small utility module containing both) and calling it from UsersRouter.handleLogIn would keep the optimistic-lock rules single-sourced.

♻️ Proposed export
 export default RestWrite;
 module.exports = RestWrite;
+module.exports.applyAuthDataOptimisticLock = applyAuthDataOptimisticLock;

Then in src/Routers/UsersRouter.js, replace the inline loop with a single call:

const { applyAuthDataOptimisticLock } = require('../RestWrite');
// ...
applyAuthDataOptimisticLock(query, user.authData, validatedAuthData);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/RestWrite.js` around lines 746 - 778, Export applyAuthDataOptimisticLock
(and its helper isLockableAuthDataValue) from RestWrite.js and remove the
duplicated loop in UsersRouter.handleLogIn by importing and calling
applyAuthDataOptimisticLock(query, user.authData, validatedAuthData);
specifically: add an export for
applyAuthDataOptimisticLock/isLockableAuthDataValue in RestWrite.js, ensure the
function signatures remain the same, and replace the inline provider/field
iteration + JSON.stringify comparison in UsersRouter.handleLogIn with the single
imported call so the optimistic-lock rules are single-sourced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@spec/vulnerabilities.spec.js`:
- Around line 5017-5020: The test stub for sendSMS currently declares the
parameter as token and returns nothing; update the sendSMS callback to accept
the mobile parameter (matching how it's called), generate or obtain an
OTP/token, assign it to the sentToken test variable, and return the expected
object shape { token, expiry } so the adapter code in src/Adapters/Auth/mfa.js
(which does const { token: sendToken, expiry } = await this.sendSMS(mobile)) can
destructure it without error.

In `@src/RestWrite.js`:
- Around line 604-610: The optimistic-lock path using
applyAuthDataOptimisticLock(this.query, originalAuthData, this.data.authData)
can cause runDatabaseOperation to rethrow a generic OBJECT_NOT_FOUND for the
losing concurrent consumer; fix by either (A) recording that an authData lock
was applied (e.g. set this._authDataOptimisticLock = true) and in
runDatabaseOperation intercept OBJECT_NOT_FOUND and rethrow a
SCRIPT_FAILED('Invalid auth data') when that flag is present, or (B) move the
authData update into this file: perform the same try/catch update attempt here
(as done around the read/update block at lines ~707-720), convert the
OBJECT_NOT_FOUND into SCRIPT_FAILED('Invalid auth data') on failure, and remove
authData from this.data before calling runDatabaseOperation so the generic path
never surface the misleading OBJECT_NOT_FOUND; reference
applyAuthDataOptimisticLock, runDatabaseOperation, this.query and
this.data.authData when applying the change.

---

Nitpick comments:
In `@src/RestWrite.js`:
- Around line 746-778: Export applyAuthDataOptimisticLock (and its helper
isLockableAuthDataValue) from RestWrite.js and remove the duplicated loop in
UsersRouter.handleLogIn by importing and calling
applyAuthDataOptimisticLock(query, user.authData, validatedAuthData);
specifically: add an export for
applyAuthDataOptimisticLock/isLockableAuthDataValue in RestWrite.js, ensure the
function signatures remain the same, and replace the inline provider/field
iteration + JSON.stringify comparison in UsersRouter.handleLogIn with the single
imported call so the optimistic-lock rules are single-sourced.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: dff31a0f-0b7d-4ac7-ae1d-446009b0506a

📥 Commits

Reviewing files that changed from the base of the PR and between 0980ab1 and 7ce0285.

📒 Files selected for processing (3)
  • spec/vulnerabilities.spec.js
  • src/RestWrite.js
  • src/Routers/UsersRouter.js

Comment thread spec/vulnerabilities.spec.js
Comment thread src/RestWrite.js Outdated
@mtrezza
Copy link
Copy Markdown
Member Author

mtrezza commented Apr 24, 2026

@coderabbitai review your feedback conversations and resolve where you found a consensus

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 24, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Let me fetch all the review comment conversations on this PR to assess where consensus has been reached.


🏁 Script executed:

#!/bin/bash
# Get all review comments on PR `#10448`
gh api repos/parse-community/parse-server/pulls/10448/comments --paginate \
  --jq '.[] | {id: .id, user: .user.login, body: .body[:300], in_reply_to_id: .in_reply_to_id, path: .path, line: .line}'

Length of output: 2582


Both conversations have reached consensus. Let me fetch the thread node IDs to resolve them.


🏁 Script executed:

#!/bin/bash
# Get review thread node IDs via GraphQL
gh api graphql -f query='
{
  repository(owner: "parse-community", name: "parse-server") {
    pullRequest(number: 10448) {
      reviewThreads(first: 20) {
        nodes {
          id
          isResolved
          comments(first: 3) {
            nodes {
              databaseId
              author { login }
              body
            }
          }
        }
      }
    }
  }
}' | jq '.data.repository.pullRequest.reviewThreads.nodes[] | {id: .id, isResolved: .isResolved, firstComment: .comments.nodes[0].databaseId, firstAuthor: .comments.nodes[0].author.login}'

Length of output: 244


Now resolving both threads.


🏁 Script executed:

#!/bin/bash
# Resolve thread 1: sendSMS false positive (spec/vulnerabilities.spec.js)
gh api graphql -f query='
mutation {
  resolveReviewThread(input: {threadId: "PRRT_kwDOAwQnRs59dB5Y"}) {
    thread { id isResolved }
  }
}'

# Resolve thread 2: inconsistent error surface addressed (src/RestWrite.js)
gh api graphql -f query='
mutation {
  resolveReviewThread(input: {threadId: "PRRT_kwDOAwQnRs59dB5e"}) {
    thread { id isResolved }
  }
}'

Length of output: 184


@mtrezza I've reviewed all comment conversations and resolved the two threads where consensus was reached:

  1. spec/vulnerabilities.spec.js — I flagged a concern about sendSMS being used incorrectly in the test. You correctly identified this as a false positive (the user-configurable sendSMS option and the adapter-internal sendSMS method are distinct), and I acknowledged the error. ✅ Resolved.

  2. src/RestWrite.js — I flagged an inconsistent error surface on the no-user UPDATE branch. You addressed this by removing the branch entirely (it was a no-op for MFA enrollment since isLockableAuthDataValue filters out the pending object), and I agreed this was the cleanest resolution. ✅ Resolved.


🧠 Learnings used
Learnt from: CR
Repo: parse-community/parse-server PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-04-01T13:35:45.477Z
Learning: Engage in review feedback: The user must engage in review of feedback comments, and not ignore or resolve a review feedback without prior engagement in a discussion and either implementing the feedback via a commit, or convincing the reviewer of their standpoint to retract the feedback.

Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2026-03-23T22:11:01.903Z
Learning: In Parse Server PRs (parse-community/parse-server), when the commit/PR type prefix is `fix:`, the PR title must describe the bug (the broken behavior), not the fix (the expected or corrected behavior). For example, instead of "fix: Strip undefined values in ParseServerRESTController to match HTTP mode" (describes the fix), prefer "fix: directAccess update with undefined field values persists them as null in MongoDB" (describes the broken behavior).

Learnt from: mtrezza
Repo: parse-community/parse-server PR: 10443
File: spec/ParseGraphQLServer.spec.js:4-8
Timestamp: 2026-04-17T00:40:13.352Z
Learning: Repo: parse-community/parse-server
PR: 10443
Files: spec/ParseGraphQLServer.spec.js (and similar spec files with a local fetch shim)
Learning: In spec files, the local fetch wrapper intentionally sets `agent: new http.Agent({ keepAlive: false })` as a default and then spreads `...options` afterward so an explicitly provided `options.agent` can override it. Do not suggest flipping the merge order to force the agent; preserving caller overrides is the intended behavior.

Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.

Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.

Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-12-02T08:00:20.138Z
Learning: For Parse Server 9 release (PR `#9938` and related), the parse/push-adapter dependency must be upgraded to version >= 8.0.0, not 7.0.0. Version 8.x drops support for Node 18.

Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:24.824Z
Learning: For Parse Server PRs, always suggest an Angular-style PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion with every new commit to the PR.

Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-12-02T06:55:53.808Z
Learning: When reviewing Parse Server PRs that add or modify Parse Server options, always verify that changes are properly reflected in three files: src/Options/index.js (where changes originate), src/Options/Definitions.js, and src/Options/docs.js. The correct workflow is: make changes in index.js first, then run `npm run definitions` to automatically replicate the changes to Definitions.js and docs.js.

Learnt from: mtrezza
Repo: parse-community/parse-server PR: 10394
File: src/Routers/FilesRouter.js:96-105
Timestamp: 2026-04-03T18:40:52.899Z
Learning: In parse-community/parse-server `src/Routers/FilesRouter.js` (PR `#10394`), the `initInfo` middleware uses `!sessionToken` (not `== null`) to gate the public-access fast path. `req.get('X-Parse-Session-Token')` returns `undefined` when the header is absent and `''` when the header is present but empty. Both values are trivially invalid session tokens; treating them identically (skipping `handleParseSession`) is intentional and correct. Using `== null` would send empty strings to `handleParseSession` for a guaranteed-failing DB lookup, which is strictly worse behavior.

Learnt from: Moumouls
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2026-03-28T19:24:55.383Z
Learning: In parse-community/parse-server (PR `#10341`), `requestComplexity.batchRequestLimit` intentionally defaults to `-1` (unlimited) in PS9. This is by design: batch request limiting is opt-in in the current major version and will become a hard default (safe non-unlimited value) in PS10, consistent with the same opt-in policy already established for other `requestComplexity` sub-options (e.g., `includeLimit`). Do not flag the `-1` default as a security gap for PS9.

Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-12-02T08:00:08.440Z
Learning: For Parse Server 9 release preparation, the parse/push-adapter dependency should be upgraded to version >= 8.0.0, not 7.x, as version 8.x is required despite dropping Node 18 support (which aligns with Parse Server 9's removal of EOL Node versions).

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/AuthDataLock.js (1)

15-19: Optional: guard against newAuthData being nullish.

Both current call sites (UsersRouter.handleLogIn and RestWrite) gate this call behind a truthy check, but the helper itself only defends against originalAuthData being nullish. A symmetric guard makes the helper safer for future callers and consistent with its own contract.

♻️ Optional defensive guard
 export function applyAuthDataOptimisticLock(query, originalAuthData, newAuthData) {
-  if (!originalAuthData) {
+  if (!originalAuthData || !newAuthData) {
     return;
   }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/AuthDataLock.js` around lines 15 - 19, The helper
applyAuthDataOptimisticLock currently only guards against originalAuthData being
nullish; add a symmetric defensive check for newAuthData (and return early if
it's nullish/undefined) to make applyAuthDataOptimisticLock(query,
originalAuthData, newAuthData) safe for future callers (similar to existing
guards at UsersRouter.handleLogIn and RestWrite) and avoid iterating
Object.keys(newAuthData) when newAuthData is nullish.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/AuthDataLock.js`:
- Around line 15-19: The helper applyAuthDataOptimisticLock currently only
guards against originalAuthData being nullish; add a symmetric defensive check
for newAuthData (and return early if it's nullish/undefined) to make
applyAuthDataOptimisticLock(query, originalAuthData, newAuthData) safe for
future callers (similar to existing guards at UsersRouter.handleLogIn and
RestWrite) and avoid iterating Object.keys(newAuthData) when newAuthData is
nullish.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5e14f9d2-359c-4c7c-bc7c-37a35d80a65b

📥 Commits

Reviewing files that changed from the base of the PR and between a0a90ff and 0a9603f.

📒 Files selected for processing (3)
  • src/AuthDataLock.js
  • src/RestWrite.js
  • src/Routers/UsersRouter.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/RestWrite.js

@mtrezza mtrezza changed the title fix: GHSA-jpq4-7fmq-q5fj v9 fix: MFA SMS one-time password accepted twice under concurrent login (GHSA-jpq4-7fmq-q5fj) Apr 26, 2026
@mtrezza mtrezza merged commit 725be0d into parse-community:alpha Apr 26, 2026
24 checks passed
parseplatformorg pushed a commit that referenced this pull request Apr 26, 2026
# [9.9.0-alpha.2](9.9.0-alpha.1...9.9.0-alpha.2) (2026-04-26)

### Bug Fixes

* MFA SMS one-time password accepted twice under concurrent login ([GHSA-jpq4-7fmq-q5fj](GHSA-jpq4-7fmq-q5fj)) ([#10448](#10448)) ([725be0d](725be0d))
@parseplatformorg
Copy link
Copy Markdown
Contributor

🎉 This change has been released in version 9.9.0-alpha.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state:released-alpha Released as alpha version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants